Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add imperative invoker updates to popover API #37133

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lukewarlow
Copy link
Contributor

Description

Add imperative invoker updates to popover API

showPopover and togglePopover can now take an options parameter.

Motivation

So people can discover this new functionality.

Additional details

Released in Chromium 133 - https://chromestatus.com/feature/5120638407409664

Related issues and pull requests

Relates to mdn/mdn#598 (this doesn't cover the anchor positioning change)

- showPopover and togglePopover can now take an options parameter.
@lukewarlow lukewarlow requested a review from a team as a code owner December 7, 2024 14:55
@lukewarlow lukewarlow requested review from wbamberg and removed request for a team December 7, 2024 14:55
@github-actions github-actions bot added the Content:WebAPI Web API docs label Dec 7, 2024
@lukewarlow
Copy link
Contributor Author

Probably needs a browser compat data update too.

@github-actions github-actions bot added the size/s [PR only] 6-50 LoC changed label Dec 7, 2024
Copy link
Contributor

github-actions bot commented Dec 7, 2024

Preview URLs

(comment last updated: 2024-12-09 01:46:06)

@lukewarlow
Copy link
Contributor Author

cc @chrisdavidmills I just noticed you mentioned working on this, so feel free to close this if you've already done it or want more detailed changes!

@hamishwillee
Copy link
Collaborator

hamishwillee commented Dec 8, 2024

@lukewarlow Howdy, is there an associated BCD update showing this is implemented anywhere?

EDIT ... and only now I see your comment. Are you planning on doing this.

Comment on lines +28 to +29
- : An {{domxref("HTMLElement")}} that triggered the popover, if any.
This provides all the same functionality that using [`popovertarget`](/en-US/docs/Web/HTML/Element/button#popovertarget) would provide.
Copy link
Collaborator

@hamishwillee hamishwillee Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I don't understand what this is for, why it is needed, or how it is used.

Say I have a button and I trigger the popover with that button, why does the popover care what triggered it. What if I lie and specify some random element, or omit this? Seems to make zero difference if it is defined or not.

I get popovertarget - that creates a link from a button (say) to the popover, but in code you create that link by ... calling this method on the thing you want want to show.

Can you create an example in both the docs that shows how this is used, and explain those things?
Also would be good to add an example in https://developer.mozilla.org/en-US/docs/Web/API/Popover_API/Using. If this is somehow like popovertarget, perhaps drawing the connection would be good.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukewarlow @chrisdavidmills Either way, this can't merge as is because IMO no one can understand the point of this without doing extra reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies I meant to respond here, while coming up with the more detailed explanation I actually found a bug in chrome's implementation so I was double checking that my understanding was correct. I'll spend some time to make this more detailed.

Fwiw here's the 4 things this does:

  • It correctly handles nested popover scenarios, if you invoke a popover from within a popover it shouldn't close it. Currently that only works declaratively, this new feature lets you do it imperatively.

  • It fixes up the focus order such that the contents of the popover are the next tab stop once opened. Currently this only works declaratively. This new feature will do this imperatively (this is the bug I found).

  • It might have an impact on light dismiss behaviour. (This might be both a spec and implementation bug).

  • Both imperative and declarative now setup an implicit anchor element for anchor positioning (this patch doesn't cover that I'd prefer if that was a follow-up)

Copy link
Collaborator

@hamishwillee hamishwillee Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lukewarlow - looking forward to seeing what you come up with. I kind of understand what you are saying in points one and two, but hard to "fully understand" without a live example.

I'm around this week but away in first 2-ish weeks of January and sporadically in between. You're welcome to seek another reviewer if I'm not around when you get to this.

@chrisdavidmills
Copy link
Contributor

cc @chrisdavidmills I just noticed you mentioned working on this, so feel free to close this if you've already done it or want more detailed changes!

Thanks @lukewarlow. I was holding off on working on this until whatwg/html#10728 was merged into the spec, but I see it has now happened. I can handle the other bits later in the month, or early next month.

@wbamberg wbamberg removed their request for review December 12, 2024 19:29
@lukewarlow lukewarlow marked this pull request as draft December 13, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants